Fix for lost clipboard contents (#5424) #5426
Conversation
gavynriebau
force-pushed
the
bugfix/clipboard-contents-lost
branch
from
a7e43c5 to
19f531f
Compare
helix-view/src/clipboard.rs
Outdated
| if cfg!(not(any(windows, target_os = "wasm32", target_os = "macos"))) { | ||
| use std::os::unix::process::CommandExt; | ||
|
|
||
| unsafe { | ||
| command_mut = command_mut.pre_exec(|| match libc::setsid() { | ||
| -1 => Err(std::io::Error::last_os_error()), | ||
| _ => Ok(()), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
setsid should be called on all unix systems:
| if cfg!(not(any(windows, target_os = "wasm32", target_os = "macos"))) { | |
| use std::os::unix::process::CommandExt; | |
| unsafe { | |
| command_mut = command_mut.pre_exec(|| match libc::setsid() { | |
| -1 => Err(std::io::Error::last_os_error()), | |
| _ => Ok(()), | |
| }); | |
| } | |
| } | |
| #[cfg(unix)] | |
| unsafe { | |
| use std::os::unix::process::CommandExt; | |
| command_mut = command_mut.pre_exec(|| { | |
| if libc::setsid() == -1 { | |
| return Err(io::Error::last_os_error()); | |
| } | |
| }); | |
| } |
For windows we might also want to pass .creation_flags(CREATE_NEW_PROCESS_GROUP)
There was a problem hiding this comment.
I've made the following changes:
- Call
setsidfor all unix systems Call.creation_flags(CREATE_NEW_PROCESS_GROUP)for windows
After making the changes I've realised the change to set CREATE_NEW_PROCESS_GROUP will never be executed as it is inside a mod provider that uses the config #[cfg(not(target_os = "windows"))].
Seeking your opinion but I am thinking of reverting back by one commit so that the PR will contain no code related to setting CREATE_NEW_PROCESS_GROUP for windows but keeps the change you suggested of calling setsid for all unix systems?
Removed changes related to setting process group for windows as they weren't relevant
Separate to this PR but I'm also considering doing a follow-up PR to cleanup the code a little, let me know if you think this would be valuable?
With the amount of conditional compilation attributes I found it slightly hard to follow what was going on andwas thinking of splitting the code out into separate files for each environment, e.g. a provider file for windows, macos, osc52, wasm, unix etc..
There was a problem hiding this comment.
I think these changes are ready for another review now.
I'm not able to change how pre_exec is called to exactly as you suggested because it needs to return a Result<()> type.
kirawi
added
C-bug
helix-view/Cargo.toml
Outdated
| @@ -22,6 +22,7 @@ helix-lsp = { version = "0.6", path = "../helix-lsp" } | |||
| helix-dap = { version = "0.6", path = "../helix-dap" } | |||
| crossterm = { version = "0.25", optional = true } | |||
| helix-vcs = { version = "0.6", path = "../helix-vcs" } | |||
| libc = "0.2" | |||
There was a problem hiding this comment.
Should this be listed under [target.'cfg(unix)'.dependencies? It probably doesn't make a difference
There was a problem hiding this comment.
Thanks, yes, you're right, it's a dependency that's, for now, only used on unix platforms. I've addressed this now.
gavynriebau
force-pushed
the
bugfix/clipboard-contents-lost
branch
2 times, most recently
from
76b0e25 to
e2b0b33
Compare
* Fix for lost clipboard contents (helix-editor#5424) * PR feedback: Call "setsid" for all unix systems * PR Feedback: Only install libc for unix targets
* Fix for lost clipboard contents (helix-editor#5424) * PR feedback: Call "setsid" for all unix systems * PR Feedback: Only install libc for unix targets
Fixes the issue described in #5424
The code has been changed so that for unix type platforms a call is made to
libc::setsidusing the unix specific CommandExt pre_exec.The
setsidcall happens just afterforkhas been called and beforeexecveis called. This means it runs in the context of the child process.The call is necessary to ensure that spawned child processes related to the clipboard (e.g.
xclip) have a distinct process session ID and process group ID.Without this change when the parent process exits, the child (i.e.
xclip), can be erroneously killed leading to copy/paste breaking in some specific circumstances such as those detailed in #5424